IGNITE-28959 Calcite. Merge Join occasionally return wrong results by zstan · Pull Request #13455 · apache/ignite

zstan · 2026-08-09T12:01:53Z

No description provided.

Copilot

Pull request overview

This PR targets an intermittent correctness issue in Calcite’s merge join execution by changing how MergeJoinNode schedules join processing and handles completion, and it extends test coverage to exercise correlated queries under different “disable join strategy” hints.

Changes:

Updated MergeJoinNode input-push/end logic and removed the previous “finishing” / checkJoinFinished() completion path.
Extended CorrelatesIntegrationTest#testCorrelatesCollision to run the same assertions under multiple join-disabling hints (to force alternate join strategies).
Removed unused Parameterized JUnit runner annotation/imports from JoinBuffersExecutionTest.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java	Alters merge join scheduling and termination logic (removes finishing state and changes when `join()` is invoked / when downstream is ended).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CorrelatesIntegrationTest.java	Re-runs correlate-collision assertions under multiple join-strategy-disabling hints.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/JoinBuffersExecutionTest.java	Removes unused Parameterized test runner annotation/imports.

Suppressed comments (5)

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:558

This join variant now ends downstream as soon as leftFinished() becomes true. For distributed joins, the class-level distributed contract indicates downstream should not be ended until both inputs are fully completed to avoid exchange inbox reopen/memory leak. Please reintroduce a finishing/draining mode for distributed execution (similar to the removed checkJoinFinished) before ending downstream.

            if (requested > 0 && leftFinished()) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:728

This termination block now ends downstream immediately when rightFinished(true) is true. In distributed mode, MergeJoinNode’s distributed javadoc indicates the node should keep draining until both inputs end (to avoid exchange inbox reopen/memory leak). Consider reinstating the prior finishing/draining behavior for distributed joins instead of ending downstream here.

            if (requested > 0 && rightFinished(true)) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:937

Full outer join now ends downstream immediately once both sides are finished, without any distributed-specific draining/cleanup path. Since the class-level distributed flag exists specifically to avoid premature downstream end when exchanges are involved, it would be safer to centralize termination/drain behavior (as the removed checkJoinFinished did) and ensure it is consistently applied across join types.

            if (requested > 0 && leftFinished() && rightFinished(true)) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:997

Semi join now ends downstream immediately when either input is finished. In distributed mode, the MergeJoinNode distributed contract indicates downstream should only be ended after both inputs complete to prevent exchange inbox reopen/memory leak. Please restore a finishing/draining path for distributed execution rather than ending downstream here.

            if (requested > 0 && (leftFinished() || rightFinished(false))) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:1060

Anti join now ends downstream immediately once leftFinished() is true. For distributed joins, the class-level distributed contract suggests the node must continue draining until both inputs end (to avoid exchange inbox reopen/memory leak). Consider reinstating the earlier finishing/draining logic rather than ending downstream here.

            if (requested > 0 && leftFinished()) {
                requested = 0;
                downstream().end();

                return;
            }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CorrelatesIntegrationTest.java:103

The test is intended to cover merge-join correctness, but using a single NO_* hint per iteration doesn’t guarantee the planner will pick MergeJoin (it can still choose another join type). This can make the regression flaky/ineffective if the plan never uses MergeJoin. Consider explicitly forcing MergeJoin via the MERGE_JOIN hint (optionally combined with the current NO_* hint).

        for (HintDefinition noHint : List.of(NO_NL_JOIN, NO_CNL_JOIN, NO_HASH_JOIN)) {
            log.info(">>> Check with: " + noHint);
            // Collision by correlate variables in the left hand.
            assertQuery("SELECT /*+ %s */ * FROM test1 WHERE ".formatted(noHint) +
                "EXISTS(SELECT * FROM test2 WHERE test1.a=test2.a AND test1.b<>test2.c) " +

ignitetcbot · 2026-08-10T05:16:32Z

TCBot Test Analysis

TeamCity: --> Run :: All Results
Branch: pull/13455/head
Base: master
Tested commit: e23d2a9

Possible Blockers (0)

No blockers found.

New Tests (0)

No new tests found.

Vladsz83 · 2026-08-10T08:47:18Z

-            "AND NOT EXISTS(SELECT * FROM test2 WHERE (SELECT test1.a)=test2.a AND (SELECT test1.b)<test2.c)")
-            .returns(12, 2)
-            .check();
+        for (HintDefinition noHint : List.of(NO_NL_JOIN, NO_CNL_JOIN, NO_HASH_JOIN)) {


Should we define which join we wanr use insteed? NO_JOIN does not guarantee another certain join wouldn't be taken twice.

seems we have numerous problems in hints, thus i can`n change it like you suggest

Vladsz83 · 2026-08-10T08:49:16Z

    protected boolean checkJoinFinished() throws Exception {
        if (!finishing) {
-            finishing = true;
+            if (leftFinished() && rightFinished(true))


What if not finished? Why we clear all then?

Vladsz83 · 2026-08-10T10:27:43Z

                    CorrelateToNestedLoopRule.INSTANCE,
-                    NestedLoopJoinConverterRule.INSTANCE,
-                    HashJoinConverterRule.INSTANCE,
+                    //NestedLoopJoinConverterRule.INSTANCE,


Let's revert

Vladsz83 · 2026-08-10T10:33:57Z

-            "EXISTS(SELECT * FROM test2 WHERE (SELECT test1.a)=test2.a AND (SELECT test1.b)<>test2.c) " +
-            "AND NOT EXISTS(SELECT * FROM test2 WHERE (SELECT test1.a)=test2.a AND (SELECT test1.b)<test2.c)")
-            .returns(12, 2)
+        for (HintDefinition noHint : List.of(MERGE_JOIN, HASH_JOIN, NL_JOIN)) {


noHint -> smth. like joinToUse

Vladsz83 · 2026-08-10T10:35:01Z

-            .returns(12, 2)
+        for (HintDefinition noHint : List.of(MERGE_JOIN, HASH_JOIN, NL_JOIN)) {
+            log.info(">>> Check with: " + noHint);
+            // Collision by correlate variables in both, left and right hands.


Let's add an empty line before

remove it at all

Vladsz83 · 2026-08-10T10:36:39Z

+        }
+    }
+
+    @Test


The checkstyle would require /** */

Vladsz83 · 2026-08-10T11:02:49Z

-            "EXISTS(SELECT * FROM test2 WHERE (SELECT test1.a)=test2.a AND (SELECT test1.b)<>test2.c) " +
-            "AND NOT EXISTS(SELECT * FROM test2 WHERE (SELECT test1.a)=test2.a AND (SELECT test1.b)<test2.c)")
-            .returns(12, 2)
+        for (HintDefinition noHint : List.of(MERGE_JOIN, HASH_JOIN, NL_JOIN)) {


Suggestion: if the hints are lost her, let's just bring a dedicated test and do not use this

already done

This reverts commit 608d017.

Vladsz83 · 2026-08-10T11:33:36Z

    }

-    /** */
-    protected boolean checkJoinFinished() throws Exception {


Suggestion: let's partly revert checkJoinFinished(), keep the cleanup and reduce many similar calls of
requested = 0; downstream().end();

I think this approach is not acceptable for all join algo, as you suggest, you can try it by yourself ... re-phrase: i think we can`t implement it as it was before cause it behavior is depend of join type used

Vladsz83 · 2026-08-10T11:55:06Z

@zstan mixed fix https://github.com/apache/ignite/pull/13459/changes , WDYT? Won't touch the distributed logic.

IGNITE-28959 Calcite. Merge Join occasionally return wrong results

e10a1ff

zstan requested a lite review from Copilot August 9, 2026 12:03

Copilot started reviewing on behalf of zstan August 9, 2026 12:04

View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Comment thread ...g/apache/ignite/internal/processors/query/calcite/integration/CorrelatesIntegrationTest.java Resolved

Comment thread ...rc/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java Resolved

fix

e23d2a9

zstan requested a lite review from Copilot August 9, 2026 13:57

Copilot started reviewing on behalf of zstan August 9, 2026 13:57

View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Vladsz83 reviewed Aug 10, 2026

View reviewed changes

fix after review

682d70f

Vladsz83 reviewed Aug 10, 2026

View reviewed changes

zstan added 3 commits August 10, 2026 14:06

fix

608d017

Revert "fix"

789bab6

This reverts commit 608d017.

fix

3c38b1e

zstan force-pushed the ignite-28959 branch from 7cc74c5 to 3c38b1e Compare August 10, 2026 11:10

Vladsz83 reviewed Aug 10, 2026

View reviewed changes

Vladsz83 approved these changes Aug 10, 2026

View reviewed changes

zstan merged commit c21741a into apache:master Aug 11, 2026
7 checks passed

+                      }
+                  }
+                  @Test

Conversation

zstan commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ignitetcbot commented Aug 10, 2026

TCBot Test Analysis

Possible Blockers (0)

New Tests (0)

Uh oh!

Vladsz83 Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vladsz83 Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vladsz83 commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Vladsz83 Aug 10, 2026 •

edited

Loading

Vladsz83 Aug 10, 2026 •

edited

Loading